feat: batch multiplex #1
Conversation
- add batch multiple feature smart contract - add its interface - update 0x interface to inherit batch multiple methods - add sample batch multiplex validator contract
- create new /examples folder - move BatchMultiplesValidator.sol to /examples
- use private method instead of copy constant
- added missing overrides - added missing parenthesis
- update scripts to include new contracts - deploy new contracts in tests/utils/DeployZeroEx.sol - update artifacts.ts and wrappers.ts files
remove input in BatchMultiplexFeature constructor in DeployZeroEx.sol
- assert meta transaction can be executed with batchMultiplex - assert onlySelf, onlyOwner modifiers not overruled by batchMultiples
- move revert transaction failed without error in if statement
- removed solved comments - added payable modifier to feature methods - added refundsAttachedEth, doesNotReduceEthBalance, onlyDelegateCall modifiers to methods - added new guard flag in reentrancy guard dep
| /// @dev Refunds up to `msg.value` leftover ETH at the end of the call. | ||
| modifier refundsAttachedEth() { | ||
| _; | ||
| uint256 remainingBalance = LibSafeMathV06.min256(msg.value, address(this).balance); |
There was a problem hiding this comment.
would this still be valid if the proxy had some eth prior to the call?
if someone sends eth to the proxy by mistake for instance
There was a problem hiding this comment.
yes, this method will refund a maximum of the amount sent, so if the proxy already holds ETH, it won't withdraw what was already in the proxy. This means that if ETH is accidentally attached to the batchMultiplex call, it will be refunded. If, on the other hand, eth was received before the call, the batchMultiplex method won't be able to transfer said ETH.
There was a problem hiding this comment.
if the proxy had eth before the call, and the amount is more than msg.value this will refund msg.value even if the swap has been successful no? bc it will be taken from previous eth
There was a problem hiding this comment.
yes, you are right! I think it would work also in the case msg.value is higher than the previous balance and sent eth is spent in the sub-calls, the proxy balance would be the previous balance plus any leftover, and all would be refunded. While technically this would not violate the onlyOwner modifier of FundRecoveryFeature, it would be executing an action reserved for the proxy owner. By moving the doesNotReduceEthBalance before this one, the transaction would revert, which is not ideal either.
| /// @dev Ensures that the ETH balance of `this` does not go below the | ||
| /// initial ETH balance before the call (excluding ETH attached to the call). | ||
| modifier doesNotReduceEthBalance() { | ||
| uint256 initialBalance = address(this).balance - msg.value; |
There was a problem hiding this comment.
since the balance could be 0, the value would be negative and so the initialBalance type needs to be signed I think
There was a problem hiding this comment.
technically that is not correct, as the proxy's ETH balance is increased by the msg.value and initialBalance is defined before calls execution. The proxy balance will always be the balance before the batchMultiplex call, plus the msg.value of the batchMultiplex call, i.e. initialBalance will always be at least as big as msg.value. Proxy ETH balance would be 0 only if msg.value is 0, which does not result in underflow. If the calc did underflow, the post-execution assertion require(initialBalance <= address(this).balance, "Batch_M_Feat/ETH_LEAK"); would revert as initialBalance would be greater than address(this).balance
| result := add(result, 0x04) | ||
| } | ||
| revert(abi.decode(result, (string))); | ||
| } else if (errorType == ErrorHandling.STOP) { |
There was a problem hiding this comment.
Would it be possible that in case of error, tokens are not properly transferred back to caller? which will make it invalid to stop/continue, as it will lead to fund loss/locking
There was a problem hiding this comment.
Technically yes, for example, if a user transferred ETH and wrapped it as first call in the batch, it would be possible for the WETH in the proxy to not be returned. In this context, the client must ensure that any excess WETH is transformed back to ETH at the end of the batch, as the proxy will then refund the excess ETH. Since the method does transfer tokens to the proxy but the underlying exchange calls do that, it should not be possible for other tokens to be stuck unless that possibility was true for the underlying methods. Will try to replicate such scenario in a test.
- refund the minimum between eth attached and the eth left in the proxy net of eth stuck in the proxy before the transaction
- add test file - update setup pipeline
- merge two batchMultiplex methods - name differently the 2 implemented methods to prevent console warning in tests and easier testing integration, using `batchMultiplex` and `batchMultiplexOptionalParams` - remove validate deploy and immutable, as moved in foundry test - use Contract.selector instead of hashing selector in migrate - use _revertWithData from ZeroEx.sol to allow decoding expected error - update interface - minor linting
- swap zrx and dai with native fill RFQ order and multiplex sell token for token to uniswap through `batchMultiplex`method
- handle edge cases of ETH transfers - added internal methods for routing - prevent weth stuck if behavior is STOP - blacklist non-supported methods - store implementations of methods that require special handling - reserve 2 EP storage slots for batch multiplex stoage - update ci pipeline
- create private method _dispatch to handle execution in both external batch execute methods - remove unnecessary variable definitions - remove eventual weth balance in both methods
- removed mapping of selector to implementations from feature storage - store mapping of selectors that require routing - storage variable name shortening - retrieve address of target implementations from proxy storage - revert on more selectors
- define only 1 mapping in storage, use enum instead of bool to define selector status - update storage initialization - move duplicate unwrap weth block to private method - added method for owner to update storage - added method to query a selector's status - rename immutable _weth to WETH
- reorder some methods according to flow: external, public, internal, private; write, read (view, pure)
- use delegatecall in transformERC20 if ETH is not the input token - use non-conditional `value` in .call as the non-ETH calls are re-routed with delegatecall - comment improvements
- create custom route as method will fail whenever eth is involved and another call using eth is executed - blacklist other limit orders - update comments
- add limit order and wrap eth tests - refactor batch multiplex tests without using non-supported meta transactions
- add new contract with non payable method for executing batch calls to ZeroEx
- renamed batch multiplex calls for better read - define a type in libs and use in the 2 batch multiplex feature contracts and interfaces - use same error behavior as pZEIP1 for uniformity
Description
Testing instructions
Types of changes
Checklist: